Repository navigation
Release 0.7.0: v1 library, CLI and docs in 9 reviewable commits - #142
BenWestgate wants to merge 9 commits into
Conversation
|
CodeQL is red, and this PR doesn't add the code it flags. The two Clearing the check needs a maintainer decision on those two alerts in code scanning, which I can't do from here. Generated by Claude Code |
04d7362 to
51c0794
Compare
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: Concept ACK to the nine-commit 0.7.0 split. Current 24 checks are green (including CodeQL); nine commits have verified signatures. Targeted security review found no immediate issue in fingerprint-before-wallet selection, no-shell Core RPC, or OS entropy. Still draft: complete an independent security review of generation, correction and wallet import; verify final artifacts and the Tails/Core restore path before release. No full security-scan ACK yet.
Rewrite the BIP93 encoding, checksum, sharing and profile layers of the v0.6.1 code (byte-identical to 4857e18, ACKed in BlockstreamResearch/codex32#74) as frozen, typed values with no third-party dependency. - gf32/checksums/bech32: immutable checksum specs, GF(32) tables and strict single-case Bech32 parsing; drop the unused segwit_addr module. - bip93: Header/Share/Secret values, parse_codex32, derive_share and recover_secret. There is no public encoder from raw (padded) bytes to a share; shares come only from interpolation. - profiles: ms (BIP32 master seed), cl (Core Lightning hsm_secret, unshared only) and bip39 entropy. - Tooling: drop mypy.ini, requirements.txt and the pylint job; add ruff, strict mypy and a hash-pinned setuptools build dependency. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Add master_xprv(), backed by a minimal private BIP32 root derivation that stops at the root; child derivation is left to Bitcoin Core. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Add generate_master_seed() and CreationCeremony, which issues shares one at a time and only finishes after each one has been read back correctly. Fresh seeds use CRC padding and the BIP32 fingerprint identifier. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Add correct() and correct_worksheet_residue(). Substitutions and erasures are decoded within the BCH bounds; insertions and deletions use a bounded alignment search. Every candidate is untrusted and carries its capture volume. Constants are checked against the frozen PR #70 corpus. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Add the deadline-bounded scheduler that proves which competing alignments cannot outrank a found candidate, so interactive callers get a ranked answer quickly. Include the alignment benchmark tool and its summary. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Add the Bitcoin Core v32 adapter used by the CLI to import a master xprv and let Core derive descriptors, plus a regtest fixture job that checks every frozen wallet fingerprint against a pinned Core release. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Add the command-line interface: argument parsing, protected terminal input with group-by-group correction, and the create, share, recover, correct and wallet commands. The production code stays within a 5200-line budget. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Rewrite the README and add the user guide, printable cards, API reference, security model and invariants, contribution guide, security policy and AI policy. Replace the Copilot instructions with AGENTS.md. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Bump the version, replace the publish workflow with a reproducible, hash-pinned build that attaches distributions to the GitHub release, and verify the installed wheel in CI. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
51c0794 to
5fc90bc
Compare
|
@codex security review Focus: share generation and read-back, correction (BCH and indel), wallet import into Bitcoin Core, and OS entropy use. Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Note for the next rebuild, once #144 ( #144 adds the private helper Suggestion: in commit 1, carry it over as private Generated by Claude Code |
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head release-gate review.
Concept ACK 5fc90bc only. Nine atomic commits stage the v1 library and CLI; the current-head automated code review reports no findings. Still draft: the full release security review, exact-artifact qualification and project-required human acceptance tests must complete before code ACK or merge. #94 is the moving preview, not the release candidate.
|
Carrying over the review from #94 before closing it as a duplicate of this PR. Both land the v1 tree on master, and this PR has the more recent review. I checked each #94 finding against this head ( Still open here
Already resolved on this head
Generated by Claude Code |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fc90bc2ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| count = len(value.replace(" ", "")) | ||
| targets, primary, reduced, _timed = _correction_plan(profile, byte_length, count, target) | ||
| deadline = monotonic() + 10 if deadline is None else deadline | ||
| contexts = tuple(CorrectionContext(profile, length, immutable, excluded) for length in targets) |
There was a problem hiding this comment.
Filter invalid target lengths before starting the CLI search
For opaque HRPs near the short/long checksum boundary, the generated target lengths include forbidden expanded lengths 94 and 95. These contexts reach _prepare(), where _checksum_for_encoded_length() raises and aborts the entire search before fixed BCH correction runs. For example, a valid 90-character zz card with one substituted checksum symbol produces expanded codex32 lengths 94 and 95 are invalid instead of a correction. Validate and skip impossible target contexts, as the public unknown-length correction path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
AI-assisted validation: confirmed at 5fc90bc. A valid 90-character opaque zz card with one checksum substitution reaches impossible 94/95 targets and raises InvalidLength before BCH. Filter unsupported expanded lengths before constructing contexts.
| prefix = context.immutable_prefix or f"{context.hrp}1" | ||
| matches = text.startswith(prefix) if context.immutable_prefix else text.lower().startswith(prefix) | ||
| if not matches or not target - 8 <= len(text) <= target + 8: |
There was a problem hiding this comment.
Match the actual separator before accepting correction context
When the input contains another 1 after the supplied context prefix, this startswith check accepts it even though Bech32 uses the last 1 as the separator. The public correction API then treats that separator as a mutable erasure and returns an artifact in a different namespace. Reproducing with a 48-character master-seed string s, replacing s[15] with 1, and calling correct(CorrectionContext('ms', expected_length=48), damaged) returns the original ms artifact despite the entered HRP extending through position 14. Reject a syntactically present HRP/separator that disagrees with the context rather than correcting it away; the mandatory boundary contract requires this context to remain immutable.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
AI-assisted validation: confirmed at 5fc90bc. Replacing position 15 with 1 still recovers the original 48-character ms seed under CorrectionContext("ms", ...), so the entered separator is being corrected away. Keep the syntactically present separator immutable.
| frontier = _frontier(states, primary) | ||
| layers_accounted = [] if capture_layers is None else capture_layers | ||
| widths = {state.target: 5 * state.degree for state in states} | ||
| layers_accounted.extend((volume, widths[key[0]]) for key, volume in frontier.items()) |
There was a problem hiding this comment.
Share admission capacity across mixed-case interpretations
For mixed-case input, the normalized and minority-erased interpretations each call _search_many(), but _frontier() starts with a fresh admission budget every time; capture_layers accumulates accounting only after admission. Consequently, their combined admitted mass can exceed the mandatory bound of 1. For example, let s = MasterSeed.from_seed(bytes(16), identifier='test').text and correct s[:10] + 'P'*13 + s[23:] with an expected length of 48: the erased interpretation admits mass 1, the normalized interpretation admits another approximately 0.318, and the public API returns a candidate whose reported cumulative mass is approximately 1.318. Admission must consume one shared budget across both interpretations, rather than merely combining their metadata afterward.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
AI-assisted validation: confirmed at 5fc90bc. The mixed-case example reports 48,626,079,622,538,851,158 / 2^65 ≈ 1.318 cumulative mass. Both interpretations must consume one admission budget.
| if not optional_only and _FIXED in state.counts: | ||
| _check_deadline(deadline) | ||
| fixed[state.target] = _search_fixed(state, frontier, results, allowed) |
There was a problem hiding this comment.
Preserve fixed witnesses during mixed-case optional searches
When mixed-case scheduling invokes this function with optional_only=True, the fixed-witness dictionary stays empty even though discovery already found candidates. _covered_candidates() interprets the missing witness as a completed, unsuccessful fixed decode and prunes optional layers that could improve a known candidate's rank. For example, encode bytes(range(16)) with identifier test, swap zero-based positions 11/12 and 23/34, and capitalize position 40: _scheduled_candidates() reports a complete search with capture volume 2,663,936, whereas the identical lowercase input finds the cheaper admitted explanation with volume 83,248. Carry the fixed witnesses into the optional pass, or distinguish an unavailable witness from a failed decode, so pruning preserves ranking rather than only discovery.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
AI-assisted validation: confirmed at 5fc90bc. The bytes(range(16)) example ranks the same source at 2,663,936 in the mixed-case optional pass versus 83,248 lowercase. Preserve the fixed witness used by competitor pruning.
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: one additional release-workflow finding.
| build: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
The build job executes repository code under python -m build --no-isolation while actions/checkout persists a contents: write token by default. Keep the build read-only; give write authority only to the release-upload boundary (or disable persisted credentials).
Requested by Ben · project thread
Before: master has the v0.6 library only (PyPI 0.6.1). The v1 work lives on
reviewability-v1as 88 commits with plans, gates and add-then-remove churn, so it's hard to review from the ACKed code.After: master gets the v1 library, the
codex32/ms32CLI and the new README as 0.7.0, in 9 atomic commits (about +18k lines). Each commit passes ruff, ruff format, strict mypy and its tests on its own.The final tree equals
reviewability-v1+ #143 (drop raw benchmark data) at e976c8c except for two differences. The version is0.7.0instead of1.0.0rc1, and master's FUNDING.yml, scorecard.yml and stale.yml are kept.Provenance. BlockstreamResearch/codex32#74 commit 4857e18 (ACKed by apoelstra) contains
src/andtests/that are byte-identical to4bb90c0(tag v0.6.1) and to the PyPI 0.6.1 sdist and wheel. Master descends from4bb90c0through 14 small CI, README and version commits. Sogit diff 4bb90c0 HEADreviews everything since the ACKed code.-O), constants, differential, build and wheel checksHow: a script takes each commit's files from the target verbatim. Only hub files get intermediate versions:
__init__.pyexports,pyproject.toml, the CI workflow, a step-1 README usage example, andindel.pywithout the competitor hook until commit 5. The script, its overlays and the verification log are in the project files underv1-rebuild/.Notes for the reviewer:
reviewability-v1.reviewability-v1work, unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K